Repository navigation
Fix wrong rows, unfiltered selects and ignored sorts in relationship queries - #3775
Merged
Merged
Conversation
… queries - A related primary-key condition on a `to` relationship no longer takes the id-filter shortcut, which compared the related key with the local key; an id filter mapped through `to` is tagged `mappedTo` and is the only kind it takes. - An array foreign key is tested per element in the shortcut. - A dangling foreign key in a to-many relationship is skipped instead of throwing. - Every AND-scope condition on a to-many relationship restricts its select(), registered when the filter is built; OR scopes keep their lazy path. Matched related records are selected by entry key, so a table whose key is not a record field works. - A multi-hop condition passes the related-level map down, so a second leading two-hop condition filters the innermost join and a non-leading one restricts the nested select(). - Sorting by an attribute across a to-many relationship resolves it per related record, bounded like loadRelatedEntries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Dispatch-Task: harper-relationship-filter-select-correctness
…usly Under asynchronous iteration, a nested selection with conditions now reads a key candidate (such as one from a leading join) with an async read and freezes it, evaluates its predicate through the async-capable form, and awaits the predicate when transformEntryForSelect rechecks it after projection. AND/OR groups of nested predicates carry a maybeAsync form, as top-level groups do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Dispatch-Task: harper-relationship-filter-select-correctness
…ants Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Dispatch-Task: harper-relationship-filter-select-correctness
…nding recheck, safe selection lookups - A sort through more than one to-many hop shares one RELATED_READS_IN_FLIGHT budget: related records resolve one at a time while a deeper to-many hop remains, instead of each hop opening its own window. - transformEntryForSelect projects a row only after a pending asynchronous recheck settles true (the projection tail is now selectRecord). - filterByType reads a query's filtered map by own property and recognizes a join Map or RelationshipSelection by type, so a relationship named like an inherited member or the maps' own bookkeeping keys no longer throws; a selection is installed only once its condition compiled. - A nested selection candidate rechecks cancellation once its read settles. - The synchronous relationship filter passes each related entry, not the array index, to the next level; an OR branch answered by the id shortcut registers its select restriction. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Dispatch-Task: harper-relationship-filter-select-correctness
…hape - A pending nested recheck that settles after the selection is returned no longer projects the row; a nested candidate checks the read-transaction guard once its read settles. - A sort through a to-many relationship stops starting related reads once its iterator is closed (the post-ordering iterator's stopped() is passed down). - transformEntryForSelect and _prepareRelationshipSelection read a query's filtered map through relationshipRestriction (own property, a join Map or an object with fromRecord), so a relationship named like a map bookkeeping key projects normally. - resources/DESIGN.md links the OR-scope select-restriction gap to harper#3762. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Dispatch-Task: harper-relationship-filter-select-correctness
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Dispatch-Task: harper-relationship-filter-select-correctness
_prepareRelationshipSelection read the query's restriction for the relationship once, when the selection was prepared. An OR-scope condition registers its restriction while the first row is filtered, after that, so a nested selection with a limit, sort or conditions ignored it while a plain select honored it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Dispatch-Task: harper-relationship-filter-select-correctness
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Dispatch-Task: harper-relationship-filter-select-correctness
Contributor
There was a problem hiding this comment.
Code Review
This pull request enhances relationship query handling by supporting asynchronous reads, select restrictions, and sorting across to-many relationships, backed by extensive new unit tests. The review feedback highlights two important improvements: first, attaching a dummy .catch() handler to promises returned by getPathValue in relatedPathValue and remainingRelatedPathValues to prevent unhandled promise rejections; second, calling checkActive?.() after matchCandidate resolves in _prepareRelationshipSelection to guarantee transaction safety after yielding to the event loop.
…whose predicate awaited Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Dispatch-Task: harper-relationship-filter-select-correctness
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Dispatch-Task: harper-relationship-filter-select-correctness
This was referenced Oct 29, 2026
kriszyp
marked this pull request as ready for review
October 30, 2026 12:57
This was referenced Oct 30, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
⊙ Problem
Relationship queries gave wrong results:
?id=p1&children.id=c1dropped the matching parent, and?id=p1&children.id=p1matched it by its own id.select()unfiltered.🤔 Is fixing all of these in one PR right?
Yes, as briefed. They share
filterByTypeand the select-restriction map it feeds. One pair of tests covers them. The async-read fix for nested selections is its own commit.💡 Solution
select()together, down to-many nested levels, and related records are matched by entry key. OR groups and to-one-then-to-many paths are not covered yet (harper#3762).sort(children.sortKey)orders parents by their related values.⚖️ Alternatives
enforceExecutionOrderand nested-selection predicates bypass the planner.🤔 Register AND restrictions when the filters are built?
Chosen over lazy registration on first use. Lazy registration made the restriction depend on which row ran first, and nested selections read it before any row ran. Side effect: an id-shortcut condition now restricts
select()too, as a leading join already did.🔧 Changes
resources/search.ts: the key-space tagmappedToon index-derived id sets; theRelationshipSelectionregistry read throughrelationshipRestriction(own properties of a known shape);inUnionthreading so OR scopes keep their old path; the sync filter passing each related entry; and the own-property lookup infilterByType.resources/Table.ts:remainingRelatedPathValueskeeps one read budget per sort path and stops once the sort iterator closes._prepareRelationshipSelectionreads its restriction on each call.transformEntryForSelectwaits for a pending recheck before projecting and reads restrictions by shape.resources/DESIGN.md: the async select and sort notes, plus the select-restriction and key-space invariants, in the relationship-query convention.🤔 Sort by a to-many attribute: element by element?
Parents compare by the array of related values in relationship order, the same way an array column sorts. Min or max per direction is the alternative. Once released, changing this changes visible order.
🤔 One extra call per projected row
The final projection step moved into
selectRecordso a pending recheck can finish first. That adds one call per row alongside the closure that step already allocates per row. I could not measure it because another session held the machine's benchmark lock.🤔 Nested selections after a cancel or a timeout
A predicate already pending when its selection is cancelled can issue one more bounded read. After a transaction timeout, the row in progress can still finish its nested selection. Those last steps do no reads, the timeout leaves the snapshot open so iterators can finish, and the commit then fails with the timeout error.
Accepted minor: relationships named
filters,fromRecordorhasMappingsget no select restriction.✅ Verification
relationshipPrimaryKeyFilters.test.js(REST +search) andrelationshipQueryContracts.test.js(read-count spies). Againstorigin/mainsources with a forced rebuild, 14 of 20 failed on rocksdb and on lmdb. Now 23/23 pass on both. The 3 cases added during review each fail on the commit before their fix./verify):?id=p1&children.id=c1→["p1"],children.id=p1→[],references.state=closed→ 200, sibling conditions narrowchildren, to-many sort orders both directions.Full gates on a loaded host
test:unit:resources(rocksdb): 5780 passing, 2 failing. Both failures pass alone (subscriptionReplay,updateAttributesLockwatchdog).subscriptionReplayexcluded. That file segfaulted in the full run and passes alone twice.test:unit:main: cut off at 58 min with turbo off. ItsdeployActivationfailures pass alone in 0.7 s.PR QA: Do relationship filters, select restrictions and to-many sorts match a brute-force oracle at scale, across pages and under concurrent child writes? — queued @ 814e555, qa-pr-harper-3775-814e555 (later heads add only a read-guard check and a DESIGN.md fold)
Docs: queued docs-harper-3775 (sorting by an attribute across a to-many relationship); the select-restriction behavior is already documented.
🤖 Generated by Claude Opus 5.5 (Claude Code); posted via @kriszyp.
Release target: main — relationship query fixes, partly in code #3279 and #3615 added on main only; no backport requested
Related PRs: #3619 overlaps (merged: same relationship read paths; the merge with main passes the relationship suites), #3724 overlaps (same mapConditionsToFilters signature), #3316 overlaps (relationship read permissions in the same selection paths), #3771 overlaps (search cancellation beside selectionStopped), 11 others independent
Complexity: complicated
Deferred findings
d1d7242cfbd2f3b2minor correctness —resources/search.ts— OR-scope preserveForSelect registers a failing branch's restriction before the match check — filed Relationship select() restrictions: OR scopes, OR members under a join, and to-one→to-many paths #37623f3d8ba1901ed9a7minor correctness —resources/search.ts— Select restriction missing for to-one hops and OR-scoped multi-hop — filed Relationship select() restrictions: OR scopes, OR members under a join, and to-one→to-many paths #37621580efaf15bc4558nit test-coverage —unitTests/resources/relationshipQueryContracts.test.js— OR select tests assert only rows[1] — NOT FILEDLate Grok findings
9 surviving (highest major) — Grok reviewed 30a3042
resources/Table.ts:8532— A cached child passes the first predicate, then its recheck goes async (pendingMatch) while a related-select projection (projected) is already running. Nothing observesprojecteduntilpendingMatchsettles. If the projection rejects first (for exampleReadSnapshotExpiredError), Node raisesresources/Table.ts:11649— Each to-many hop opens its own window of 16 reads (RELATED_READS_IN_FLIGHT), so two to-many hops in one sort path (children.siblings.name) put 16×16 = 256 reads in flight.resources/DESIGN.md:269says the cap still holds after a to-many hop. The wide-sort test has only one to-many hop, so it cresources/search.ts:1807— The selection object has afilterskey and is also passed down as the next level's relationship map. For a to-many relationship namedfilters, a non-leading condition likechildren.filters.flagreads that metadata array asfilterMapand adopts it as the selection.selection.filters.pushatresources/Table.ts:7653— Cancellation is not rechecked after an awaited read:unitTests/resources/relationshipQueryContracts.test.js:1— The test usesrequire('node:assert/strict'), which house style prohibits. Switch to plainnode:assertwith explicitstrictEqual/deepStrictEqualresources/search.ts:1890— In the sync relationship filter,subObject.some(nextFilter)passes the array index asentry, and thesubEntriesit builds are thrown away (deferred to a follow-up)resources/search.ts:1806— No select restriction is applied for to-one→to-many hops, OR multi-hop conditions, or OR branches that take the id shortcut (deferred to a follow-up)resources/DESIGN.md:270— The note documents the OR-scope "first evaluated condition wins" gap but links no tracking issueresources/Table.ts:11648— New comments that narrate the code, aggregated into one nit:Not standing: none. Grok ran 27m (27m from launch to report); the fold adjudication (
cursor-grok@30a30424172f-c9897035, claude) took 1m.Late Grok findings
1 surviving (highest minor) — Grok reviewed 3623247, ruled against 0e7b7ae
resources/Table.ts:7545— When a relationship filter sits inside an OR and the select also passesconditions,sort,limitoroffset, the select is not narrowed by that filter._prepareRelationshipSelectionreads the filter's restriction once, atresources/Table.ts:7545, while the select step is being built (`resoNot standing: none. Grok ran 23m (24m from launch to report); the fold adjudication (
cursor-grok@36232476e2f1-f71b1bb8, claude) took 1m.Late Grok findings
5 surviving (highest minor) — Grok reviewed 12d1f4a, ruled against 814e555
resources/search.ts:1870— a to-one hop (noelements) and any OR-scoped multi-hop register no select restriction.preserveForSelectatresources/search.ts:1904-1907stores only the outer predicate, soselectstill returns related rows that the condition rejected. This is already deferred as `select-restriction-missingresources/search.ts:1870— a to-many relationship namedfilters,fromRecordorhasMappingsis excluded bySELECTION_BOOKKEEPING(resources/search.ts:1536), sorestrictsSelectis false. Example: with related rows g1 (code=yes) and g2 (code=no),filters.code = yesprojects both rowsresources/Table.ts:7659— cancellation is applied only aftermatchCandidatesettles. If a cancel lands while a nested predicate (for example aloadEntriesongrand.great.code) is pending, its storage reads still run to completion and hold a background read slot until they settleresources/Table.ts:7778— four continuations resume without callingcheckActive:unitTests/resources/relationshipQueryContracts.test.js:360— the plain OR select test (:360) and the OR test with a limit (:380) assert onlyrows[1].rows[0](or-a, whose child c7 matched) is unasserted because it currently returnschildren: []under the deferred first-wins behaviour. A regression on or-a stays greenNot standing: 1 separate-issue, 2 merged. Grok ran 16m (16m from launch to report); the fold adjudication (
cursor-grok@12d1f4a0ccf8-3d79cb00, claude) took 1m.Late Grok findings
5 surviving (highest minor) — Grok reviewed 39fcf53
resources/Table.ts:7660— when a nested candidate predicate returns false after the transaction has timed out, the code returnsSKIPbeforecheckActiveruns. If that is the last candidate, the parent row is produced withouttransactionOpenTooLongError. The join scan atresources/search.ts:1512-1513checks the guard wresources/Table.ts:7782— the offset and projection continuations check onlystopped, which stays false onabortDueToTimeout. They then push orfinish()with no read guard, so the last row lands without the timeout errorresources/Table.ts:7659— cancellation is seen only aftermatchCandidatesettles, so a nested predicate's in-flight related reads keep running afterclose(). This is bounded to one predicate evaluation, and the resume returnsSKIPresources/search.ts:1870— a to-many relationship namedfilters,fromRecordorhasMappingsgets no select restriction, so non-matching related rows are projected. This is documented inresources/DESIGN.md:394. The test atunitTests/resources/relationshipQueryContracts.test.js:451-461has only one related row, so itunitTests/resources/relationshipQueryContracts.test.js:360— both OR select tests (lines 360 and 380) assert onlyrows[1]. The wrongor-a.childrenresult ([], although childc7matched) is invisible to them. That gap is the deferred first-wins restriction defect, tracked in harper#3762Not standing: 1 separate-issue. Grok ran 11m (11m from launch to report); the fold adjudication (
cursor-grok@39fcf537c09b-c7ed3f1d, claude) took 1m.Origin — the dispatch brief this PR was written from
Fix relationship-path filter/select/sort defects in resources/search.ts (filterByType ~1784-1895, preserveForSelect ~1858-1882, attributeComparator) and resources/Table.ts (_prepareRelationshipSelection ~7529-7675, getPathValue ~11556-11592). From F-1046: (1) the recordFilter idFilter shortcut (~1828-1845, and its maybeAsync copy ~1891) builds subIdFilter from resolver.from ?? primaryKey even when resolver.to is set, comparing the related PK against the wrong field: guard with (!resolver.to || idFilter.idSet); (2) allowObjectMatching=true makes an array FK (from: childIds) match as a whole array: use !resolver.toMany || Array.isArray(relatedTable.primaryKey); (3) getSubObject's subEntry.map(e => e.value) throws on a dangling FK (HTTP 500): skip nullish entries. From F-1051: (4) preserveForSelect binds fromRecord only for the first non-leading condition (!filtered[firstAttributeName]); accumulate siblings; (5) fromRecord uses value[relatedTable.primaryKey], undefined for composite keys: use entry.key ?? value[pk]; (6) the recursive call passes filterMap?.[name]?.joined (~1794) instead of the sibling map; (7) getPathValue does value?.[attr] on an Array for to-many relationships on the path: map over elements; (8) readCandidate calls getEntry without async: true / freezeRelatedEntry and compile drops maybeAsync (predicate() called synchronously), violating the invariant #3615 wrote into resources/DESIGN.md. If (8) grows large, land it as a second commit in the same PR.
Acceptance
Both staged tests pass on rocksdb and lmdb (P-6647 asserts async-read counts for item 8); existing relationship suites stay green. Show each new test failing on origin/main sources (forced rebuild) before the fix.
Dispatch: task
harper-relationship-filter-select-correctness· queued by finding-triage/wave-43 · ran by claude/opus/xhigh · worker kzyp-xps-1Review-Coverage: authored=claude; ran=cursor-composer,gemini,codex,cursor-grok; late=cursor-grok:adjudicated; adjudicated=domain; declined=muse; rounds=8; full=2 @ 516a50e
Review-Attention: deep ~50m (critical: Table.ts, search.ts; decisions: cancellation-granularity, reserved-relationship-names, or-scope-first-wins, do-less-alternative) @ 516a50e